Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding Physical Switch power action #392

Merged
merged 1 commit into from
Jun 20, 2018

Conversation

douglasgabriel
Copy link
Member

@douglasgabriel douglasgabriel commented Jun 7, 2018

This PR is able to

  • Add the restart action to the /physical_switches endpoint

Depends on
ManageIQ/manageiq-providers-lenovo#177 - Merged
ManageIQ/manageiq#17548 - Merged

@douglasgabriel douglasgabriel force-pushed the ph_switch_pw_op branch 2 times, most recently from 2a59de6 to 2667e70 Compare June 7, 2018 18:18
@miq-bot
Copy link
Member

miq-bot commented Jun 7, 2018

Checked commit douglasgabriel@1d52463 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

@douglasgabriel
Copy link
Member Author

I'm closing and reopen this PR just to re-execute the Travis 👍

@douglasgabriel
Copy link
Member Author

@miq-bot assign @imtayadeway

@abellotti abellotti self-assigned this Jun 11, 2018
@abellotti abellotti removed their assignment Jun 11, 2018
@agrare
Copy link
Member

agrare commented Jun 19, 2018

@miq-bot assign @gtanzillo

@gtanzillo gtanzillo self-assigned this Jun 20, 2018
@gtanzillo gtanzillo requested a review from abellotti June 20, 2018 08:41
@gtanzillo
Copy link
Member

@abellotti Can you take a look at this one and let me know if the look ok

#
# Enqueues the action for multiple resources.
# For multiple resources, when an error occurs, the error messages must
# be built individually for each resource. Always responding with status 200.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@douglasgabrieIt's not obvious what the difference is between enqueue_action_single_resource and enqueue_action_multiple_resources. Can you provide some more details?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! Basically a single resource request failure will look like:

[status = 404]
{
    "error": {
        "kind": "not_found",
        "message": "Couldn't find PhysicalSwitch with 'id'=4 [WHERE \"switches\".\"type\" IN ('PhysicalSwitch', 'ManageIQ::Providers::Lenovo::PhysicalInfraManager::PhysicalSwitch')]",
        "klass": "ActiveRecord::RecordNotFound"
    }
}

However, a multiple resource request failure will look like:

[status = 200]
{
    "results": [
        {
            "success": true,
            "message": "Performing restart for Physical Switch id:1 name: 'Lenovo Switch'",
            "task_id": "2",
            "task_href": "http://localhost:3000/api/tasks/2",
            "href": "http://localhost:3000/api/physical_switches/1"
        },
        {
            "success": false,
            "message": "Couldn't find PhysicalSwitch with 'id'=9999 [WHERE \"switches\".\"type\" IN ('PhysicalSwitch', 'ManageIQ::Providers::Lenovo::PhysicalInfraManager::PhysicalSwitch')]"
        }
    ]
}

The main difference here is that for a single resource request, the entire response fails, see the status.

But, for multiple resources, the failures are self contained, once some resources could execute the action successfully and others could fail, see the status again.

This is why the enqueue_action_multiple_resources method has a rescue for these errors and inside it a graceful message is mounted for the user, which doesn't occurs in enqueue_action_single_resource.

It made the things more clear for you @gtanzillo ? Maybe add those examples in the method documentation would be a good option, what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that helps a lot. One more question -

How is id passed to enqueue_action_multiple_resources? Is it one id at a time or an array of ids?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One at time

@gtanzillo gtanzillo added this to the Sprint 89 Ending Jul 2, 2018 milestone Jun 20, 2018
@gtanzillo gtanzillo merged commit c059f38 into ManageIQ:master Jun 20, 2018
@miq-bot
Copy link
Member

miq-bot commented Jul 3, 2018

@douglasgabriel 'imtayadeway' is an invalid assignee, ignoring...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants